Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tools: Enable Unit Testing for SITL #18404

Merged
merged 8 commits into from
Sep 27, 2021

Conversation

hendjoshsr71
Copy link
Member

This enables units tests to be run by CI when built under SITL. The reasoning for this came from PR #18365 & #18363 where I was trying to fix simulated drivers and spent far to long in the debugger when a unit test would have worked perfectly.

It enables GTest to catch exit events such as an abort.
Thus we can test that SITL creates Internal_Errors where required and prevent regressions as a bonus.

There are some details, such as the method to detect if the unit tests or running inside the Panic(), that I'm not sure will work across all platforms?

Log from the "test unit tests ...." tab in this PR showing the Panic: AP_Internal_Error being printed and caught by GTest for evaluation.
image

@hendjoshsr71 hendjoshsr71 changed the title Enable Unit Testing for SITL Tools: Enable Unit Testing for SITL Aug 20, 2021
tests/AP_gtest.h Outdated Show resolved Hide resolved
@hendjoshsr71 hendjoshsr71 force-pushed the pr/sitl_unit_testing branch 2 times, most recently from b41dbe7 to 7f7a254 Compare August 23, 2021 08:32
@hendjoshsr71 hendjoshsr71 force-pushed the pr/sitl_unit_testing branch 2 times, most recently from a3da5c8 to 7a8f183 Compare August 29, 2021 09:02
@hendjoshsr71
Copy link
Member Author

@peterbarker

I switched this to use WEAK symbols now and removed the function you weren't happy about.
I do think like Buzz said on the call it may make sense to just build and do unit tests against SITL alone instead of Linux?

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Reduce a bit the coverage stats but it will allow to cover the missing functions, so that is great!

I think that having the weak symbol on panic for SITL and Linux is fine

it doesn't impact Chibios builds according to tests

@hendjoshsr71
Copy link
Member Author

Reduce a bit the coverage stats but it will allow to cover the missing functions, so that is great!

@khancyr Could you explain to me how it reduces the coverage stats? I'm learning about coverage in general so I'm interested to figure this out.

@khancyr
Copy link
Contributor

khancyr commented Sep 20, 2021

As we are now using SITL on coverage, we are passing over lines that were restricted to SITL only before. Those are mostly some panic and debug lines. But we don't necessary check them as there weren't there on Linus build. Therefore we test more stuff but check less outputs so our coverage statistic is dropping.

You can see it easily on the coverage report

@hendjoshsr71
Copy link
Member Author

Ahh makes sense. So you are telling me I need to add some more tests to get your coverage stats back up ;)

I ran the coverage report for the only test that is SITL only SIM_MS5611. They were quite useful to find that one of my tests in that wasn't testing a few lines they were supposed to be.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me.

My understanding is this would've helped with some testing that was being done as part of some sim refactoring.

@khancyr what do you think?

@khancyr
Copy link
Contributor

khancyr commented Sep 26, 2021

I have tested and it is working. So that is fine with me.

@tridge tridge merged commit 1019628 into ArduPilot:master Sep 27, 2021
@hendjoshsr71 hendjoshsr71 deleted the pr/sitl_unit_testing branch September 28, 2021 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants